Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add periodic_task_name #261

Merged
merged 5 commits into from
Dec 15, 2021
Merged

feat: add periodic_task_name #261

merged 5 commits into from
Dec 15, 2021

Conversation

lvelvee
Copy link

@lvelvee lvelvee commented Nov 18, 2021

@lvelvee
Copy link
Author

lvelvee commented Nov 18, 2021

related: https://github.com/lvelvee/django-celery-beat/tree/feat-periodic-task-name

auvipy
auvipy requested changes Nov 18, 2021
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check the test failures

@lvelvee lvelvee changed the title feta: add periodic_task_name feat: add periodic_task_name Nov 18, 2021
@lvelvee lvelvee requested a review from auvipy November 18, 2021 14:45
@auvipy
Copy link
Member

auvipy commented Nov 18, 2021

some documentation change is require i guess

@auvipy
Copy link
Member

auvipy commented Nov 22, 2021

I am curious what extra benefits we get by adding this? what the existing 2 packages are missing?

@lvelvee
Copy link
Author

lvelvee commented Nov 23, 2021

I am curious what extra benefits we get by adding this? what the existing 2 packages are missing?

If a CeleryTask may correspond to more than one PeriodicTask, it will not be possible to distinguish the source of the PeriodicTask because django-celery-results lacks the name field of the PeriodicTask.

For example, if I have two PeriodicTask named periodic_a and periodic_b, both of which call the test task of celery at regular intervals, without this patch, the results will only be known as the results of test, and there will be no way to distinguish whether the task originated from periodic_a or periodic_b.

@lvelvee
Copy link
Author

lvelvee commented Dec 15, 2021

@auvipy Is it still possible to be merged?

auvipy
auvipy approved these changes Dec 15, 2021
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess some change in celery beat is also required?

@auvipy auvipy merged commit 5e67a31 into celery:master Dec 15, 2021
@lvelvee
Copy link
Author

lvelvee commented Dec 15, 2021

I'll make the pr later

@lvelvee lvelvee mentioned this pull request Dec 15, 2021
Merged
auvipy pushed a commit to celery/django-celery-beat that referenced this pull request Dec 17, 2021
feat: add periodic_task_name in favor of celery/django-celery-results…
77a6ccf 
…#261 (#477)

* feat: add periodic_task_name

* docs and tests

* more detail docs

Co-authored-by: mba <[email protected]>
@lvelvee lvelvee deleted the patch-periodic-task-name branch December 17, 2021 08:02
@auvipy auvipy linked an issue Dec 22, 2021 that may be closed by this pull request
Store task name ? #7
Closed
@KOliver94 KOliver94 mentioned this pull request Jan 9, 2022
Closed
@KOliver94 KOliver94 mentioned this pull request Jan 26, 2022
Merged
@awais786 awais786 mentioned this pull request May 13, 2022
Merged
@peterfarrell peterfarrell mentioned this pull request Sep 26, 2022
Closed
Oleh929216 added a commit to Oleh929216/Django-beat that referenced this pull request Jan 30, 2025
@Oleh929216
feat: add periodic_task_name in favor of celery/django-celery-results…
ea42f05 
…#261 (#477)

* feat: add periodic_task_name

* docs and tests

* more detail docs

Co-authored-by: mba <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auvipy auvipy auvipy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store task name ?
2 participants
@lvelvee @auvipy